Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Fixed cleanup of control plane logging lab #709

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

ROunofF
Copy link
Contributor

@ROunofF ROunofF commented Oct 20, 2023

What this PR does / why we need it: Ignore cleanup error in observability labs, in case logs were never enabled.

Which issue(s) this PR fixes:

Fixes #696

Quality checks

  • My content adheres to the style guidelines
  • I ran make test module="observability" and it was successful

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@netlify
Copy link

netlify bot commented Oct 20, 2023

Deploy Preview for eks-workshop ready!

Name Link
🔨 Latest commit ac52fb3
🔍 Latest deploy log https://app.netlify.com/sites/eks-workshop/deploys/658613f2dec2420008d5c983
😎 Deploy Preview https://deploy-preview-709--eks-workshop.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ROunofF ROunofF marked this pull request as ready for review October 20, 2023 21:27
@niallthomson
Copy link
Contributor

I think the logging logic probably needs to be more complex than this. If we want to be able to implement something that tells the user when a command failed and which log file there will need to be something that catches the error since set -e will exit. Also, it looks like this is redirecting stdout as well is that correct?

@ROunofF
Copy link
Contributor Author

ROunofF commented Oct 25, 2023

Also, it looks like this is redirecting stdout as well is that correct?

This will capture (in the log) and display both stdout and stderr. Since we have set -e, in the observability I added the || true since we know the error is possible.

I didn't change the logic, except we have that log file of the last action that trigged the problem the issue will see (but it still could be caused by a previous call)

@niallthomson
Copy link
Contributor

Oh yeah, forgot its tee.

What I mean by the logic is that in the script that curls reset-environment, runs and it and uses tee ideally we would catch any error that happens in the reset-environment run and message the user to refer to appropriate log file by exact path.

For example

$ prepare-environment mymodule
[...]
An error occurred, please contact your workshop proctor or raise an issue at https://github.com/aws-samples/eks-workshop-v2/issues
Log file can be found at /eks-workshop/logs/action[...].log

@ROunofF ROunofF marked this pull request as draft October 26, 2023 14:50
@niallthomson niallthomson changed the title Added logging options and fix observability cleanup feat: Added logging options and fix observability cleanup Oct 26, 2023
@ROunofF ROunofF marked this pull request as ready for review October 26, 2023 16:20
@ROunofF
Copy link
Contributor Author

ROunofF commented Oct 30, 2023

Oh yeah, forgot its tee.

What I mean by the logic is that in the script that curls reset-environment, runs and it and uses tee ideally we would catch any error that happens in the reset-environment run and message the user to refer to appropriate log file by exact path.

For example

$ prepare-environment mymodule
[...]
An error occurred, please contact your workshop proctor or raise an issue at https://github.com/aws-samples/eks-workshop-v2/issues
Log file can be found at /eks-workshop/logs/action[...].log

Have a look at the latest PR, this should be pretty similar to your suggestions

@niallthomson niallthomson changed the title feat: Added logging options and fix observability cleanup chore: Added logging options and fix observability cleanup Nov 20, 2023
@tceren
Copy link

tceren commented Dec 19, 2023

hi team, what are we missing to continue merging this pull request? happy to help!

@niallthomson niallthomson changed the title chore: Added logging options and fix observability cleanup chore: Fixed cleanup of control plane logging lab Dec 22, 2023
@niallthomson
Copy link
Contributor

I've removed the logging feature from this PR for now so we can get the basic fix merged.

@niallthomson niallthomson merged commit bc10611 into aws-samples:main Dec 22, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants